refactor: bundling external fallbacks with webpack #637
Conversation
packages/one-app-bundler/__tests__/webpack/plugins/register-external-injector.spec.js
Outdated
Show resolved
Hide resolved
| @@ -15,9 +15,11 @@ | |||
| import chalk from 'chalk'; | |||
| import fs from 'node:fs'; | |||
| import path from 'node:path'; | |||
| import snakeCase from 'lodash.snakecase'; | |||
There was a problem hiding this comment.
Can we just implement this here instead of adding a dependency (esp lodash)
function toSnakeCase(str) {
return str
.replace(/[\s-]+/g, '_') // Replace spaces and dashes with _
.replace(/([A-Z])/g, '_$1') // Prefix uppercase letters with _
.toLowerCase() // Convert the whole string to lowercase
.replace(/^_/, ''); // Remove leading underscore if present
}There was a problem hiding this comment.
why can't we just go with lodash? we used it before in the dev bundler and does not affect the output/bundle
There was a problem hiding this comment.
We already have all of lodash we don't need lodash.snakecase specifically as well right?
packages/one-app-bundler/webpack/externalFallbacks/webpack.client.js
Outdated
Show resolved
Hide resolved
| resolve: { | ||
| mainFields: MAIN_FIELDS, | ||
| modules: [packageRoot, 'node_modules'], | ||
| extensions: ['.js', '.jsx', '.ts', '.tsx'], |
There was a problem hiding this comment.
| extensions: ['.js', '.jsx', '.ts', '.tsx'], | |
| extensions: ['.js', '.jsx', '.ts', '.tsx', '.cjs'], |
There was a problem hiding this comment.
i don't think this is needed since it's strange to have client side code using commonjs
There was a problem hiding this comment.
@giulianok I had to fix this last week with the latest redux release, so it is needed
#633
| module: { | ||
| rules: [ | ||
| { | ||
| test: /[jt]sx?$/, |
There was a problem hiding this comment.
| test: /[jt]sx?$/, | |
| test: /\.c?[jt]sx?$/, |
There was a problem hiding this comment.
i don't think this is needed since it's strange to have client side code using commonjs
There was a problem hiding this comment.
packages/one-app-bundler/webpack/externalFallbacks/webpack.client.js
Outdated
Show resolved
Hide resolved
packages/one-app-bundler/webpack/externalFallbacks/webpack.server.js
Outdated
Show resolved
Hide resolved
packages/one-app-bundler/webpack/externalFallbacks/webpack.server.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Matthew Mallimo <[email protected]>
Co-authored-by: Jamie King <[email protected]>
…ps://github.com/americanexpress/one-app-cli into refactor/external-fallbacks-bundle-with-webpack
| fs.writeFileSync(configPath, JSON.stringify({ | ||
| ...config, | ||
| ...newData, | ||
| }, null, 2)); |
There was a problem hiding this comment.
Can we save some bytes by not pretty printing the json?
There was a problem hiding this comment.
i don't think it will make a huge diff but why not
| util: resolve('util'), | ||
| vm: resolve('vm-browserify'), | ||
| zlib: resolve('browserify-zlib'), | ||
| }, |
There was a problem hiding this comment.
If we are pollyfilling this ^ all of these modules in fallback: elsewhere, does it make sense to do it again here within the externalFallbacks?
There was a problem hiding this comment.
https://github.com/americanexpress/one-app-cli/blob/main/packages/one-app-bundler/webpack/module/webpack.client.js These are pollyfills that were provided by webpack 4 and no longer are so we manually pollyfill them here. probably should not provide it again? maybe my understanding of what actually is happening in the externalFallbacks version is off
There was a problem hiding this comment.
not sure if I follow.
we aren't actually pollyfilling, we provide a fallback, which only uses the pollyfil if the actual lib cannot be used / isn't available. We do it for the modules and we need to do it for external fallbacks since they are separate bundles
There was a problem hiding this comment.
mm okay I think im mainly just trying to figure out if this work is being duplicated. Because in module/webpack.client.js we pollyfill, and here we fallback the pollyfill it seems so if the pollyfill is found it should not duplicate such work
There was a problem hiding this comment.
@giulianok I think Danny's point is that this code is repeated. Maybe we can extract this out
10xLaCroixDrinker
deleted the
refactor/external-fallbacks-bundle-with-webpack
branch
Provide a general summary of your changes in the Title above.
Bundling external fallbacks with
webpackinstead ofesbuildDescription
Describe your changes in detail
External Fallbacks are currently being bundled with
esbuild. There are certain externals that use libs such ascryptothat require a fallback to a polyfill (e.g.crypto-browserify) in order to properly work on the browser.esbuildsupports it viaaliasesbut only for the code that's being transpiled and not for its dependencies.webpacksolves by via thefallbackflag which tries to resolve the dependency and defaults to a specified one in case it cannot, even for dependencies.We could have created an
esbuildplugin to implement thefallbackfeature fromwebpack, but we decided to instead usewebpackto bundle it to be consistent with production and because we are unsure whenesbuildwill be used for production bundles.This project only accepts pull requests related to open issues.
If suggesting a new feature or change, please discuss it in an issue first.
Motivation
Why is this change required? What issue does it resolve?
Test Conditions
Describe in detail how the changes are tested.
Include details of your testing environment, and the tests you ran to.
How does your change affect the rest of the code.
Types of changes
Check boxes that apply:
Checklist
Check boxes that apply: